Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EnsureAssetsCompiled feature #222

Merged
merged 2 commits into from
Jan 25, 2016
Merged

Add EnsureAssetsCompiled feature #222

merged 2 commits into from
Jan 25, 2016

Conversation

robwise
Copy link
Contributor

@robwise robwise commented Jan 24, 2016

Review on Reviewable

@justin808
Copy link
Member

Looking good!


Reviewed 7 of 7 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


docs/additional_reading/rspec_configuration.md, line 2 [r1] (raw file):
If you did use stale Webpack assets, you will get invalid test results as your tests do not use the very latest JavaScript code.


docs/additional_reading/rspec_configuration.md, line 4 [r1] (raw file):
@samnang Please review. I'm pretty sure we need to pass config.


docs/additional_reading/rspec_configuration.md, line 6 [r1] (raw file):
and only if you do not have the webpack -w processes that would build the generated files.


docs/additional_reading/rspec_configuration.md, line 8 [r1] (raw file):
per my prior statement. I would not mention TDD workflow.

If you do not want to be slowed down by re-compiling webpack assets from scratch every test run, you can call ...

README.md, line 379 [r1] (raw file):
👍


Comments from the review on Reviewable.io

@robwise
Copy link
Contributor Author

robwise commented Jan 24, 2016

Review status: all files reviewed at latest revision, 4 unresolved discussions.


docs/additional_reading/rspec_configuration.md, line 2 [r1] (raw file):
Done.


docs/additional_reading/rspec_configuration.md, line 6 [r1] (raw file):
see below I address that next


docs/additional_reading/rspec_configuration.md, line 8 [r1] (raw file):
what prior statement?


Comments from the review on Reviewable.io

@justin808
Copy link
Member

:lgtm_strong: 🎉 👏


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

justin808 added a commit that referenced this pull request Jan 25, 2016
@justin808 justin808 merged commit 8c3604d into master Jan 25, 2016
@robwise robwise deleted the ensure-assets-compiled branch January 25, 2016 15:59
@samnang
Copy link
Contributor

samnang commented Jan 26, 2016

docs/additional_reading/rspec_configuration.md, line 4 [r1] (raw file):
@justin808 @robwise Great stuff. I have few comments on this:

  1. instead of doing before(:example), we should use before(:suite).
  2. Instead letting users of our gem to know about the method and we need to expose that, we just have lib/react_on_rails/rspec file helper, so the user just require that file react_on_rails/rspec in their rails helper to have this integration helper.

Comments from the review on Reviewable.io

@robwise
Copy link
Contributor Author

robwise commented Jan 26, 2016

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


docs/additional_reading/rspec_configuration.md, line 4 [r1] (raw file):
@samnang

  1. If I do before suite, the thing will compile webpack bundles even if the user is just doing a quick unit test that has nothing to do with webpack.
  2. We need access to the RSpec config, so we have to have the user pass it as a variable. Also, there was an issue where if we have auto-executing code on require, then this runs twice (once on requiring of the gem by Rails, once on requiring in the rails helper)

Comments from the review on Reviewable.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants